Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Compat API] Also print successfully tagging images in /build endpoint #9524

Merged
merged 1 commit into from
Mar 11, 2021
Merged

[Compat API] Also print successfully tagging images in /build endpoint #9524

merged 1 commit into from
Mar 11, 2021

Conversation

riyad
Copy link
Contributor

@riyad riyad commented Feb 25, 2021

Trying to find regressions by exercising the APIv2 trough docker-py's test suite (see #5386) I came across a bug in the BuildTest::test_build_in_context_dockerfile test (there're a few tests failing because of the same issue).

There're several tests that check for Docker's "summary lines" (e.g. "Successfully ...") in the output stream of the /build endpoint.

Example output from Docker's /build endpoint:

{"stream":"Step 1/4 : FROM busybox"}
{"stream":"\n"}
{"stream":" ---\u003e 491198851f0c\n"}
{"stream":"Step 2/4 : ENV FOO=bar"}
{"stream":"\n"}
{"stream":" ---\u003e Using cache\n"}
{"stream":" ---\u003e d8a2bf82d287\n"}
{"stream":"Step 3/4 : RUN touch baz"}
{"stream":"\n"}
{"stream":" ---\u003e Using cache\n"}
{"stream":" ---\u003e bd627c1ed937\n"}
{"stream":"Step 4/4 : RUN touch bax"}
{"stream":"\n"}
{"stream":" ---\u003e Using cache\n"}
{"stream":" ---\u003e 18cabba43a7d\n"}
{"aux":{"ID":"sha256:18cabba43a7d7523a29f4a1f63e4c5796930e301f01b4b013bacb13ecfd597a8"}}
{"stream":"Successfully built 18cabba43a7d\n"}
{"stream":"Successfully tagged docker_compat_test:latest\n"}

Building the same container with Podman:

{"stream":"STEP 1: FROM busybox\n"}
{"stream":"STEP 2: ENV FOO=bar\n"}
{"stream":"STEP 3: RUN touch baz\n"}
{"stream":"STEP 4: RUN touch bax\n"}
{"stream":"STEP 5: COMMIT docker_compat_test\n"}
{"stream":"Getting image source signatures\n"}
{"stream":"Copying blob sha256:84009204da3f70b09d2be3914e12844ae9db893aa85ef95df83604f95df05187\n"}
{"stream":"Copying blob sha256:087b0e9db0ecf766354b7c48d7ca9ae9ab93843fd97dcfc51c1685443536ec26\n"}
{"stream":"Copying config sha256:4d3709bf17edc199ce3999904a2fa050dad849d4a75e1e6e2061c8d8ccf6d046\n"}
{"stream":"Writing manifest to image destination\n"}
{"stream":"Storing signatures\n"}
{"stream":"--\u003e 4d3709bf17e\n"}
{"stream":"4d3709bf17edc199ce3999904a2fa050dad849d4a75e1e6e2061c8d8ccf6d046\n"}
{"stream":"Successfully built 4d3709bf17ed\n"}

Note that the current code already mimics Docker's behavior by additionally sending the "Successfully built ..." line when using the compat API.
This PR "superficially" improves compatibility with Docker by also adding the "Successfully tagged ..." messages.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2021
@riyad riyad changed the title [Compat API] Also print succesfully tagging images in /build endpoint [Compat API] Also print successfully tagging images in /build endpoint Feb 25, 2021
@riyad riyad force-pushed the apiv3-print-tags-when-building branch 3 times, most recently from 11ad785 to 4a96d6e Compare February 25, 2021 20:06
@riyad
[Compat API] Also print successfully tagging images in /build endpoint
ba319e3 
[NO TESTS NEEDED]

Signed-off-by: Riyad Preukschas <riyad@informatik.uni-bremen.de>
@riyad riyad force-pushed the apiv3-print-tags-when-building branch from 4a96d6e to ba319e3 Compare February 25, 2021 20:06
@riyad
Copy link
Contributor Author

riyad commented Feb 25, 2021 •
edited
Loading

Should I keep the changes to the swagger docs?

@rhatdan
Copy link
Member

rhatdan commented Feb 25, 2021

Yes.
You either need to add tests, or set the NO TESTS NEEDED flag.

@riyad riyad marked this pull request as ready for review February 25, 2021 21:32
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2021
@riyad
Copy link
Contributor Author

riyad commented Mar 2, 2021

@rhatdan done.
I'd appreciate if you could have another look.

@rhatdan
Copy link
Member

rhatdan commented Mar 2, 2021

LGTM

@riyad
Copy link
Contributor Author

riyad commented Mar 11, 2021

@rhatdan Is there anything I can/should do?

@mheon
Copy link
Member

mheon commented Mar 11, 2021

@riyad Just commenting so the maintainers see it is plenty - sometimes we lose track of things.

This LGTM, but I'd like a signoff from @baude or @jwhonce before merge.

@mheon
Copy link
Member

mheon commented Mar 11, 2021

/approve

@baude
Copy link
Member

baude commented Mar 11, 2021

lgtm

@baude baude added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, riyad

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jwhonce
Copy link
Member

jwhonce commented Mar 11, 2021

@riyad Thank you for your work

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021
@jwhonce jwhonce self-requested a review March 11, 2021 16:54
@openshift-merge-robot openshift-merge-robot merged commit 8d33bfa into containers:master Mar 11, 2021
@riyad
Copy link
Contributor Author

riyad commented Mar 12, 2021

Thanks 😄

@riyad riyad deleted the apiv3-print-tags-when-building branch March 12, 2021 22:02
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwhonce jwhonce Awaiting requested review from jwhonce

Assignees

@jwhonce jwhonce

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@riyad @rhatdan @mheon @baude @openshift-ci-robot @jwhonce @openshift-merge-robot